test: harden reliable test execution#699
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR hardens local and CI test execution with serialized heavy-run locks, offline provider safeguards, isolated production Playwright runs, deterministic property-test seeds, exact JUnit-based flake classification, revised CI gates, and expanded verification documentation and tests. ChangesTest execution and runner safety
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CI
participant PlaywrightRunner
participant NextServer
participant JUnitReport
participant FlakeLedger
CI->>PlaywrightRunner: start production UI job
PlaywrightRunner->>NextServer: build and start isolated production app
NextServer-->>PlaywrightRunner: report identity and route readiness
PlaywrightRunner->>JUnitReport: run journeys and write failure identities
CI->>JUnitReport: read failed testcases
JUnitReport->>FlakeLedger: match exact spec and title
FlakeLedger-->>CI: classify quarantine or investigation
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@copilot resolve the merge conflicts in this pull request |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b518c1de90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isWindows = process.platform === "win32"; | ||
| const baseScripts = ["check:runtime", "format:check", "lint", "typecheck", "test"]; | ||
| const projectRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); | ||
| const baseScripts = ["check:runtime", "format:changed", "lint", "typecheck", "test"]; |
There was a problem hiding this comment.
When verify:pr-local is invoked with its documented --files option, readScope() correctly classifies that list, but this unconditional format:changed entry re-runs ci-change-scope without the list. For example, --files scripts/run-playwright.mjs selects one path while the formatter sees all 59 branch changes, so unrelated formatting can fail a scoped preflight; conversely, on a clean checkout it may check no supplied path at all. Pass the explicit selection through to the formatting command (or make the formatter consume it) so the selected verification plan is consistent.
Useful? React with 👍 / 👎.
Head branch was pushed to by a user without write access
Resolved and pushed in commit I merged |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/run-playwright.mjs (1)
161-192: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftWait for the owned server to exit before releasing the lock.
On non-Windows systems,
SIGTERMis asynchronous, but cleanup immediately deletes the output and releases the shared lock. Another heavy run can start while the previous Next process is still alive. Await process exit with a bounded escalation path before removing output and releasing the lock.As per coding guidelines, “Run only one heavy database command at a time across worktrees.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run-playwright.mjs` around lines 161 - 192, Update cleanup() and the owned-server shutdown flow around stopOwnedProcessTree() to wait for the server process and its tree to exit before deleting absoluteRunRoot and releasing lock. Make shutdown asynchronous, add a bounded wait with escalation if SIGTERM does not finish promptly, and preserve the existing Windows taskkill behavior and cleanup warning handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci-change-scope.mjs`:
- Line 149: Update the coveragePatterns regular expression to include
run-playwright.mjs as an independently coverage-sensitive runner. Ensure the
coverage classification works when run-playwright is the only changed runner,
without relying on run-vitest or changing the existing runner matching.
In `@scripts/flake-ledger.mjs`:
- Around line 22-26: Update dateValue to validate that the parsed date preserves
the input year, month, and day, rejecting impossible calendar dates such as
February 31 before returning the timestamp. Keep the existing YYYY-MM-DD format
validation and error behavior for invalid dates.
- Around line 57-60: Update loadFlakeLedger to reject parsed ledger data when
raw.flakes is missing or is not an array, rather than substituting an empty
array. Preserve validation through validateFlakeLedgerEntries for valid arrays,
and surface a clear failure for malformed ledger structure.
In `@scripts/test-environment.mjs`:
- Around line 46-51: Update the environment construction in the test-environment
helper so overrides are applied before the final fail-closed scrubbing. Ensure
protected provider-access settings, including OPENAI_API_KEY,
ALLOW_PROVIDER_TESTS, and RAG_PROVIDER_MODE, cannot be restored or replaced by
overrides while preserving the required offline/demo values.
In `@scripts/test-focused.mjs`:
- Around line 9-10: Update unsafeSelectionPattern to match Playwright
configuration files, including playwright.config.ts and
playwright.visual.config.ts, so changes to Playwright test infrastructure fail
closed and require the full suite while preserving existing unsafe-path matches.
In `@tests/flake-ledger.test.ts`:
- Around line 14-15: Update the isoDate helper and related expiry assertions in
the flake-ledger tests to use one fixed timestamp rather than Date.now(). Pass
that same timestamp through the validator’s now option so date offsets remain
stable across UTC midnight and preserve the intended acceptance and rejection
cases.
- Around line 69-75: Add a substring-only case to the test around
validateFlakeLedgerEntries: make the referenced spec contain a longer title or
comment that includes the ledger title but not as an exact test identity, then
assert validation throws the existing “exact title is not present” error.
Preserve the existing exact-title success case and missing-title failure case.
In `@tests/property-seed.ts`:
- Around line 3-8: Update the FAST_CHECK_SEED validation around parsedSeed to
reject partially numeric values such as “123abc”. Validate that the complete
environment string represents a valid integer before converting it, while
preserving the defaultPropertySeed fallback and existing propertySeed
normalization for valid inputs.
---
Outside diff comments:
In `@scripts/run-playwright.mjs`:
- Around line 161-192: Update cleanup() and the owned-server shutdown flow
around stopOwnedProcessTree() to wait for the server process and its tree to
exit before deleting absoluteRunRoot and releasing lock. Make shutdown
asynchronous, add a bounded wait with escalation if SIGTERM does not finish
promptly, and preserve the existing Windows taskkill behavior and cleanup
warning handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06a94b5c-c322-42a0-99b1-84cd536550b9
📒 Files selected for processing (58)
.github/workflows/ci-triage.yml.github/workflows/ci.yml.gitignoreAGENTS.mdREADME.mddocs/branch-review-ledger.mddocs/codebase-index.mddocs/process-hardening.mddocs/testing.mdnext.config.tspackage.jsonplaywright.config.tsplaywright.visual.config.tsscripts/check-format-changed.mjsscripts/check-rag-fixtures.mjsscripts/child-process-result.mjsscripts/ci-change-scope.mjsscripts/classify-playwright-failures.mjsscripts/eval-rag-offline.mjsscripts/flake-ledger.mjsscripts/playwright-base-url.tsscripts/run-heavy.mjsscripts/run-live-tests.mjsscripts/run-playwright.mjsscripts/run-vitest.mjsscripts/test-environment.mjsscripts/test-focused.mjsscripts/test-rag-offline.mjsscripts/test-run-lock.mjsscripts/verify-pr-local.mjssrc/instrumentation.tssrc/proxy.tstests/architecture-boundaries.test.tstests/client-secret-surface.test.tstests/demo-data.test.tstests/flake-ledger.jsontests/flake-ledger.test.tstests/instrumentation.test.tstests/property-accessible-table.test.tstests/property-chunking.test.tstests/property-numeric-token-preservation.test.tstests/property-seed.tstests/proxy.test.tstests/rag-answer-fallback.test.tstests/test-runner-safety.test.tstests/tsx-server-only-runner.test.tstests/ui-accessibility.spec.tstests/ui-formulation.spec.tstests/ui-smoke.spec.tstests/ui-stress.spec.tstests/ui-tools-collapse.spec.tstests/ui-tools-task-directory.spec.tstests/ui-tools.spec.tstests/ui-visual-artifacts.spec.tstests/universal-search-owner.live.test.tstests/universal-search.test.tstests/verify-pr-local.test.tsvitest.config.mts
💤 Files with no reviewable changes (1)
- tests/ui-visual-artifacts.spec.ts
| "tests", | ||
| "package.json", | ||
| "vitest.config.mts", | ||
| /^scripts\/(?:child-process-result|run-heavy|run-live-tests|run-vitest|test-environment|test-focused|test-run-lock)\.mjs$/, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Classify run-playwright.mjs independently as coverage-sensitive.
Line 149 omits run-playwright from coveragePatterns. The test at Lines 385-389 passes both runners together, so run-vitest masks the omission. A Playwright-runner-only PR therefore skips the coverage job despite the asserted contract.
Also applies to: 385-389
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ci-change-scope.mjs` at line 149, Update the coveragePatterns regular
expression to include run-playwright.mjs as an independently coverage-sensitive
runner. Ensure the coverage classification works when run-playwright is the only
changed runner, without relying on run-vitest or changing the existing runner
matching.
| function dateValue(value, field, id) { | ||
| if (!/^\d{4}-\d{2}-\d{2}$/.test(value)) throw new Error(`${id}: ${field} must be YYYY-MM-DD`); | ||
| const parsed = Date.parse(`${value}T23:59:59Z`); | ||
| if (!Number.isFinite(parsed)) throw new Error(`${id}: ${field} is not a valid date`); | ||
| return parsed; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject impossible calendar dates instead of relying on Date.parse.
Date.parse("2026-02-31T23:59:59Z") normalizes the value rather than returning NaN, so malformed ledger dates can pass validation and distort ordering or expiry checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/flake-ledger.mjs` around lines 22 - 26, Update dateValue to validate
that the parsed date preserves the input year, month, and day, rejecting
impossible calendar dates such as February 31 before returning the timestamp.
Keep the existing YYYY-MM-DD format validation and error behavior for invalid
dates.
| export function loadFlakeLedger(sourcePath = ledgerPath) { | ||
| const raw = JSON.parse(readFileSync(sourcePath, "utf8")); | ||
| return validateFlakeLedgerEntries(Array.isArray(raw.flakes) ? raw.flakes : []); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail when the ledger’s flakes field is missing or not an array.
The current fallback validates [], allowing malformed JSON such as {} to pass self-test and silently disable all known-flake classifications.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/flake-ledger.mjs` around lines 57 - 60, Update loadFlakeLedger to
reject parsed ledger data when raw.flakes is missing or is not an array, rather
than substituting an empty array. Preserve validation through
validateFlakeLedgerEntries for valid arrays, and surface a clear failure for
malformed ledger structure.
| return { | ||
| ...environment, | ||
| RAG_PROVIDER_MODE: "offline", | ||
| NEXT_PUBLIC_DEMO_MODE: "true", | ||
| ...overrides, | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent overrides from restoring provider access.
Because overrides is spread last, callers can restore OPENAI_API_KEY, set ALLOW_PROVIDER_TESTS=true, or replace RAG_PROVIDER_MODE. Apply overrides before scrubbing, or reject protected override keys, so this helper remains fail-closed.
As per coding guidelines, “Do not use external APIs, paid services, credentials, secrets, live Supabase projects, or OpenAI provider calls unless explicitly authorized.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-environment.mjs` around lines 46 - 51, Update the environment
construction in the test-environment helper so overrides are applied before the
final fail-closed scrubbing. Ensure protected provider-access settings,
including OPENAI_API_KEY, ALLOW_PROVIDER_TESTS, and RAG_PROVIDER_MODE, cannot be
restored or replaced by overrides while preserving the required offline/demo
values.
Source: Coding guidelines
| const unsafeSelectionPattern = | ||
| /^(?:tests\/|scripts\/|\.github\/|package(?:-lock)?\.json$|tsconfig|vitest\.config|next\.config|eslint)/i; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat Playwright configuration as unsafe focused-test input.
The pattern misses playwright.config.ts and playwright.visual.config.ts. Those test-infrastructure changes can therefore run vitest related instead of failing closed and requiring the full suite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-focused.mjs` around lines 9 - 10, Update unsafeSelectionPattern
to match Playwright configuration files, including playwright.config.ts and
playwright.visual.config.ts, so changes to Playwright test infrastructure fail
closed and require the full suite while preserving existing unsafe-path matches.
| function isoDate(offsetDays: number) { | ||
| return new Date(Date.now() + offsetDays * 24 * 60 * 60 * 1000).toISOString().slice(0, 10); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Freeze now instead of deriving expiry cases from the wall clock.
If these calls straddle UTC midnight, isoDate(31) becomes only 30 days ahead when validation runs and the rejection assertion can fail intermittently. Use one fixed timestamp and pass it through the validator’s now option.
Also applies to: 60-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/flake-ledger.test.ts` around lines 14 - 15, Update the isoDate helper
and related expiry assertions in the flake-ledger tests to use one fixed
timestamp rather than Date.now(). Pass that same timestamp through the
validator’s now option so date offsets remain stable across UTC midnight and
preserve the intended acceptance and rejection cases.
| it("requires the exact title to exist in the referenced spec", () => { | ||
| const entry = validEntry(); | ||
| const root = temporarySpec(`test(${JSON.stringify(entry.title)}, () => {});`); | ||
| expect(validateFlakeLedgerEntries([entry], { root })).toHaveLength(1); | ||
| expect(() => validateFlakeLedgerEntries([validEntry({ title: "missing @quarantine" })], { root })).toThrow( | ||
| /exact title is not present/, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exercise a substring title to enforce the claimed exact-match contract.
The current implementation validates with spec.includes(flake.title), so a ledger title appearing only inside a longer test title or comment passes this test. That permits validated ledger entries which can never exactly match the JUnit identity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/flake-ledger.test.ts` around lines 69 - 75, Add a substring-only case
to the test around validateFlakeLedgerEntries: make the referenced spec contain
a longer title or comment that includes the ledger title but not as an exact
test identity, then assert validation throws the existing “exact title is not
present” error. Preserve the existing exact-title success case and missing-title
failure case.
| const defaultPropertySeed = 424_242; | ||
| const parsedSeed = Number.parseInt(process.env.FAST_CHECK_SEED ?? `${defaultPropertySeed}`, 10); | ||
| if (!Number.isSafeInteger(parsedSeed)) { | ||
| throw new Error(`FAST_CHECK_SEED must be a safe integer; received ${process.env.FAST_CHECK_SEED}.`); | ||
| } | ||
| const propertySeed = ((parsedSeed % 2_147_483_647) + 2_147_483_647) % 2_147_483_647 || defaultPropertySeed; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject partially numeric seed values.
Number.parseInt("123abc", 10) silently produces 123, so malformed FAST_CHECK_SEED values do not trigger the promised validation. Validate the complete string before conversion to preserve deterministic replay semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/property-seed.ts` around lines 3 - 8, Update the FAST_CHECK_SEED
validation around parsedSeed to reject partially numeric values such as
“123abc”. Validate that the complete environment string represents a valid
integer before converting it, while preserving the defaultPropertySeed fallback
and existing propertySeed normalization for valid inputs.
|
Superseded by merged PR #705 ( |
Summary
Verification
npm run verify:pr-localDuring development, use
npm run verify:cheapas the faster iteration gate before the final PR-local preflight.npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changednpm run verify:releasebefore release or handoff confidence claimsFor retrieval, ranking, selection, chunking, source/citation rendering, or answer-contract changes,
verify:pr-localrunseval:rag:offlineautomatically. Run the offline command directly during iteration before spending a live eval.npm run eval:retrieval:quality(must stay 36/36) when retrieval, ranking, selection, chunking, or scoring behavior changed — CI cannot run it (needs live keys), so run it locally and paste the summary. A metadata/governance-weighting change once buried correct docs (recall 1.0→0.76) and only this eval caught it.npm run eval:rag -- --limit 15+npm run eval:quality -- --rag-onlywhen answer generation, the synthesis prompt, or answer post-processing changed (grounded-supported must not drop; citation-failure 0)npm run check:production-readinesswhen clinical workflow, privacy, environment, Supabase, source governance, or deployment behavior changednpm run check:deployment-readinesswhen deployment startup, hosting, or rollout behavior changedClinical Governance Preflight
Complete this section when the change touches ingestion, answer generation, search/ranking, source rendering, document access, privacy, production env, or clinical output.
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
Summary by CodeRabbit
Bug Fixes
New Features
Documentation